Helpers cleanup #1237
Merged
Helpers cleanup #1237
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JustinDrake
added
the
milestone:June 30 freeze 🥶
CarlBeek
reviewed
Jun 30, 2019
There was a problem hiding this comment.
This is a much more sensible ordering of helpers, I really like it! I do have a few comments though:
- For consistency's sake, I think the
Terminologysection should be removed from1_custody.mdtoo. - I am not a fan of the function naming used in the
Miscsection of0_beacon_chain.md. Based on the name alone, I don't think I would be able to determine what a function does in many of the cases. See my individual comments for details. If you are averse to the use ofgetin function names in this section, what about usingcomputeorcalculateinstead (this is already done incompute_committee).
JustinDrake
commented
Jun 30, 2019
|
I tried making |
|
I think we should leave as is instead of the "default" value here. It is a configurable constant that might be changed in the future (or in different deployments) |
|
I'd prefer to revert to my last commit and get this merged. We have other things to handle that are waiting on this PR |
That's fine :) (Tried moving it to see if spec builder would be happier.) Making it |
djrtwo
approved these changes
Jun 30, 2019
This was referenced Jul 1, 2019
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #918. It includes: